fix: Update config diff check method to handle nested arrays #7579

camdecoster · 2025-10-10T22:41:16Z

Description

Update config diff check to handle nested arrays.

Closes #7572.

Changes

Linting/formatting
Update config diff check method to handle nested arrays
Move method to helpers file
Add tests for method

Testing

Be on master

Run this mock in plotly.js DevTools:

{
  "data": [
    {
    "error_x": {
      "array": [100000.0, 100000.0, 100000.0],
      "type": "data",
      "visible": true
    },
    "type": "scatter",
    "x": [1554370371547.085, 1554370471547.085, 1554370571547.085],
    "y": [6, 10, 2]
    }
  ],
  "layout": {
    "xaxis": {
    "type": "date"
    }
  },
  "config": {
    "modeBarButtons": [[]]
  }
}

Be aware that entering the next command will cause an infinite recursive loop, so add a debugger statement in react before you do to avoid crashing your browser/computer
Call react from the browser DevTools console with the following command:
```
Plotly.react(gd, gd.data, gd.layout, { modeBarButtons: [ [] ] })
```
Click the debugger continue button a number of times and note that it just keeps happening
Switch to this branch
Run through the same process again
Click the debugger continue button and note that it only runs through once

Notes

The current diff method doesn't handle nested arrays properly, so the diff check will always return true in this case
It didn't cause a problem before because the old code ran through react once and that was it. The new code can make a call to react recursively, which is triggered by the above example.
It only happens with the modeBarButtons config option because that's the only one that takes a nested array as a value

TODO

Add draftlog

emilykl · 2025-10-14T15:41:54Z

src/plot_api/helpers.js

    return false;
 };

+const AX_LETTERS = ['x', 'y', 'z'];


put at top of file perhaps? I would normally expect constants not inside a function to be defined at the top of the file.

emilykl · 2025-10-14T16:00:00Z

src/plot_api/helpers.js

+ * @param {Object|Array} oldCollection: Old version of collection to compare
+ * @param {Object|Array} newCollection: New version of collection to compare
+ */
+const hasCollectionChanged = (oldCollection, newCollection) => {


just a nit, but I would expect this function to be called something slightly more general like areObjectsEqual() since the function itself doesn't care whether oldCollection and newCollection are versions of the same object or not; that's a matter of how the function is used.

I was trying to be generic with the term "collection" since you could be passing in an object or an array, but we could go further as you suggest. Would areCollectionsEqual be acceptable?

yes, areCollectionsEqual (or maybe collectionsAreEqual?) sounds good to me 👍

Actually, one related question — what is the reason for ignoring keys starting with an underscore? Is that due to a JavaScript implementation detail, or a Plotly.js one? Maybe that should be captured in the function name (since it's a very particular definition of equality)

That's a plotly.js detail, but it's also a JavaScript convention. Variables that start with underscore are labeled as such because they are meant to be used internally by the library. So, you wouldn't see a user changing those values, hence they can be ignored.

emilykl · 2025-10-14T16:36:54Z

src/plot_api/helpers.js

+        if (Object.keys(oldCollection).length !== Object.keys(newCollection).length) return true;
+
+        for (const k in oldCollection) {
+            if (k.startsWith('_')) return false;


this seems wrong, am I missing something? Is it supposed to be if (k.startsWith('_')) continue; ?

Wait, looking closely this whole loop is problematic; it should only ever return true from within the loop. Line 535 will return false sometimes and short-circuit the whole loop if hasCollectionChanged(oldVal, newVal) returns false.

Good catch! I did a poor job of converting this loop from it's previous iteration. I updated it to only return when true.

emilykl · 2025-10-14T16:41:15Z

@camdecoster Infinite loop is resolved, but I am not able to update the modebar buttons successfully by calling Plotly.react().

This is the line I am running in the browser console:

Plotly.react(gd, gd.data, gd.layout, { modeBarButtons: [['zoom2d', 'pan2d'], ['autoScale2d', 'resetScale2d']] });

I would expect to see new buttons show up in the modebar but I see nothing. I am also seeing that hasCollectionChanged() seems to return false in that case but I haven't dug deeper than that.

emilykl · 2025-10-14T17:27:35Z

Also have you been able to learn anything about the intentions behind this test? Unclear whether it was deliberately forbidden for a philosophical reason, or just because it was complicated to implement.

camdecoster · 2025-10-14T18:47:53Z

Also have you been able to learn anything about the intentions behind this test? Unclear whether it was deliberately forbidden for a philosophical reason, or just because it was complicated to implement.

I've only looked at it to understand why it was failing. In this case, it was because of the bad loop conversion. I'll see if I can understand it a bit better and report back.

emilykl · 2025-10-14T18:51:48Z

I've only looked at it to understand why it was failing. In this case, it was because of the bad loop conversion. I'll see if I can understand it a bit better and report back.

I ask because the wording of the test case ("should not try to transition when the config has changed") seems to directly oppose the functionality you've added (making sure a transition happens during react() when the config has changed). I don't see any reason why that's a problem, but the person who added the test originally apparently thought that shouldn't happen.

camdecoster · 2025-10-14T23:56:50Z

I ask because the wording of the test case ("should not try to transition when the config has changed") seems to directly oppose the functionality you've added (making sure a transition happens during react() when the config has changed). I don't see any reason why that's a problem, but the person who added the test originally apparently thought that shouldn't happen.

Okay, so config changes shouldn't be animated per this comment. The second call to react that I added handles the situation where both the config and some attribute that should be animated were changed. On the second pass through, the config isn't changing, so that should trigger the transition (if one should occur).

So, the test is there to make sure no transitions happen when a config change occurs. This might actually add a new feature that didn't exist before the changes to react in #7475: now you can pass in changes to the config and other transition-triggering changes and they should both get handled.

camdecoster · 2025-10-15T00:30:48Z

Infinite loop is resolved, but I am not able to update the modebar buttons successfully by calling Plotly.react().

This was a consequence of the loop logic error. I tried it after the fix and it worked. Could you try again with the latest changes?

camdecoster added 4 commits October 10, 2025 15:27

Linting/formatting

b480d04

Move diff function to helpers and update to handle nested arrays

34a669e

Add tests for diff function

364466a

Switch object clone method to keep undefined values in clone

3b3816f

camdecoster changed the title ~~Cam/7572/update-config-diff-check~~ fix: Update config diff check method to handle nested arrays Oct 10, 2025

camdecoster marked this pull request as ready for review October 10, 2025 22:50

camdecoster requested a review from emilykl October 10, 2025 22:50

camdecoster assigned alexshoe and camdecoster Oct 10, 2025

camdecoster added 3 commits October 13, 2025 09:51

Switch loops to improve performance

20c534f

Fix test label

47be541

Add draftlog

ef102cc

emilykl reviewed Oct 14, 2025

View reviewed changes

camdecoster added 2 commits October 14, 2025 12:39

Fix bad loop conversion

1b65938

Move constant to top of file; remove unused require

0da832e

camdecoster added 2 commits October 14, 2025 18:28

Switch method to check for equality

2767f07

Update tests per method change

01e698a

Uh oh!

fix: Update config diff check method to handle nested arrays #7579

Are you sure you want to change the base?

fix: Update config diff check method to handle nested arrays #7579

Conversation

camdecoster commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing

Notes

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilykl Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilykl commented Oct 14, 2025

Uh oh!

emilykl commented Oct 14, 2025

Uh oh!

camdecoster commented Oct 14, 2025

Uh oh!

emilykl commented Oct 14, 2025

Uh oh!

camdecoster commented Oct 14, 2025

Uh oh!

camdecoster commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

camdecoster commented Oct 10, 2025 •

edited

Loading

emilykl Oct 14, 2025 •

edited

Loading